runtime: add attribute formatting functionality in FluentLocalization #212
Conversation
eemeli
left a comment
eemeli
left a comment
There was a problem hiding this comment.
Extending format_value() like this would be rather misleading, as the added functionality is specifically about formatting a message attribute, not its value.
Instead, I think we should add a new method format_message(), which takes the same arguments as format_value(), but returns a tuple of the formatted value (or None, if there is no value), together with a dict of all of the formatted attributes, keyed by their names.
This connects with another Fluent design choice: attributes should not be used separately, but only as a part of the whole message.
Also, we do need to include tests and a documentation update when making an API change like this.
|
Sounds fair enough, I'll try to implement that soon. Anyway, I think that added functionality could be added too (with tests and docs, of course). According to the function name it should format any kind of values, even it is an attribute of a message, because attributes also have their own values. |
Never mind. Pattern resolving works not as I thought, so I refuse the idea. Just will implement what you've mentioned above |
loudblow
changed the title
eemeli
left a comment
eemeli
left a comment
There was a problem hiding this comment.
Looks promising! See inline for detail-y comments.
| for bundle in self._bundles(): | ||
| if not bundle.has_message(msg_id): | ||
| continue | ||
| msg = bundle.get_message(msg_id) |
There was a problem hiding this comment.
Why not something like this?
| for bundle in self._bundles(): | |
| if not bundle.has_message(msg_id): | |
| continue | |
| msg = bundle.get_message(msg_id) | |
| msg = next(( | |
| bundle.get_message(msg_id) | |
| for bundle in self._bundles() | |
| if bundle.has_message(msg_id) | |
| ), None) |
There was a problem hiding this comment.
The first reason is consistency, format_value also uses a for loop. The second reason is simplicity: the code looks linear and less bloated to those seeing it for the first time.
| return FormattedMessage( | ||
| # Never FluentNone when format_pattern called externally | ||
| cast(str, val), | ||
| formatted_attrs if formatted_attrs else {}, | ||
| ) | ||
| return FormattedMessage(msg_id, {}) |
There was a problem hiding this comment.
Let's just return a (val, formatted_attrs) tuple rather than defining a new class.
There was a problem hiding this comment.
This is a regular tuple with attributes. Some users may prefer to pass a single DTO in their code instead of passing the message and attributes separately. Using fmt_msg.message and fmt_msg.attributes seems more intuitive than fmt_msg[0] and fmt_msg[1]
2 participants
Resolving issue with accessing message attributes via FluentLocalization.format_value("message.attribute")
Connected issues:
#209